feat(storage): pluggable dump storage backends#7
Conversation
- Add internal/storage leaf: a backend-neutral Store interface (Put/Get/Delete/List/Stat, context-aware) with a shared ErrNotFound sentinel and a RunStoreSuite contract test every backend runs. - Ship two backends: local (temp-write+rename atomic publish, keys map verbatim to file names so existing catalogs need no migration) and s3 (AWS SDK v2 transfer-manager streaming upload, atomic on completion, path-style for S3-compatible endpoints like MinIO/R2). - Refactor dumps.Catalog to hold a Store and address dumps by key instead of filesystem paths; drop Path/MetaPath/Root. Catalog methods are now context-aware. backup/restore/verify/dumps stream through the store; SHA-256 integrity stays app-side over envelope++body, so a dump verifies identically whether local or in S3. - Add a storage: config block (type local|s3, bucket, prefix, region, endpoint) with fail-fast validation; S3 credentials resolve from the standard AWS chain, never stored in config. - Cover the local backend with the contract suite plus atomic-publish and key-layout unit tests; cover the s3 backend with the same contract suite against MinIO via testcontainers under the integration tag. - Document the feature in docs/STORAGE.md; update README and CHANGELOG.
|
Warning Review limit reached
More reviews will be available in 48 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPhase G adds a backend-neutral ChangesPhase G: Pluggable Storage Backend for Dump Catalog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config.go`:
- Around line 43-50: The Validate method of StorageConfig does not trim
whitespace from the Type and Bucket fields before validation, allowing values
with only whitespace to pass validation and fail later at runtime. At the
beginning of the Validate method, trim whitespace from both s.Type and s.Bucket
fields using strings.TrimSpace, then proceed with the existing validation logic.
This ensures that storage.type and storage.bucket values containing only spaces
are rejected immediately rather than deferring failure to runtime.
In `@internal/dumps/catalog.go`:
- Around line 152-156: The order of deletion operations in the delete method is
incorrect and risks leaving dangling catalog entries on partial failure.
Currently, the code deletes the dump file first using dumpKey(id) and then the
metadata using metaKey(id). Reverse the order of these two Delete calls so that
the metadata is deleted first using metaKey(id), then the dump file is deleted
using dumpKey(id). This ensures that if the second delete fails, the worst case
is an orphaned dump body rather than a catalog entry pointing to a deleted body.
- Around line 136-139: In the List method, the current error handling after
calling ReadMeta skips all errors indiscriminately using continue, which masks
backend failures and context cancellation errors. Instead, differentiate between
error types: only continue for corrupt entry errors that should be skipped by
the user later, but return immediately for system errors like context.Canceled
or other backend failures. Check the error type or message to determine if it
represents a corrupt user entry before deciding whether to continue or propagate
the error.
In `@internal/storage/local_test.go`:
- Around line 38-39: The os.ReadDir call on line 38 ignores the returned error
using underscore, which can mask setup failures and result in misleading
assertion messages when the directory is checked for cleanliness. Capture the
error returned by os.ReadDir instead of discarding it, then add explicit error
handling (typically using t.Fatal or t.Errorf in the test) to properly report if
the ReadDir operation fails before proceeding with the len(entries) != 0
assertion.
In `@internal/storage/local.go`:
- Around line 58-66: The Put method fails to guarantee durable writes as
declared in the interface because it only closes and renames the temporary file
without syncing data to disk. After writing to the temporary file (tmp), call
Sync on the file handle before closing it to ensure all file data is written to
disk. Additionally, after the os.Rename call that moves tmpName to its final
location p, open and sync the parent directory to ensure the metadata changes
from the rename operation are persisted, which will satisfy the durability
guarantee.
- Around line 62-63: The os.Rename call in the Store.Put method fails on Windows
when the destination file already exists, violating the overwrite contract
required by the interface. To fix this, implement a platform-safe atomic replace
strategy by first removing the destination file (using os.Remove on the path p)
before attempting os.Rename, or use a cross-platform helper function that
handles both Windows and Unix-like systems. This ensures that overwriting an
existing key works consistently across all platforms in the CI matrix, including
windows-latest.
In `@internal/storage/store.go`:
- Around line 18-23: The documentation for the ErrNotFound variable incorrectly
states that Get/Stat/Delete all return ErrNotFound when a key is absent, but
this contradicts the interface contract where Delete and Stat are defined as
non-errors for missing items. Update the documentation comment for ErrNotFound
to accurately reflect that only Get returns ErrNotFound for missing keys,
removing the incorrect references to Stat and Delete from the description.
In `@internal/storage/suite.go`:
- Around line 125-126: The io.ReadAll call at line 125 and the Stat call at line
140 are ignoring errors by using blank identifiers, which can mask real backend
failures in the suite tests. Capture the error return values from both
io.ReadAll and Stat operations, and add appropriate error assertions or checks
(such as asserting the error is nil or handling the error case) to ensure these
operations succeed before proceeding with the contract assertions. This will
prevent the tests from passing when there are actual backend failures.
In `@internal/tui/modals/restore.go`:
- Line 30: The call to d.Dumps.List() in the restore modal uses
context.Background() which creates an unbounded context that can block the TUI
indefinitely during remote storage operations (e.g., S3). Replace
context.Background() with a bounded context that has a timeout, or refactor the
NewRestore function to accept a context parameter from the caller and thread
that context through to the List() call. This ensures the storage I/O operation
is cancellable and won't hang the user interface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77f7ee94-5658-43e0-9b25-f5cc4e7ead49
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
CHANGELOG.mdREADME.mddocs/STORAGE.mdgo.modinternal/app/backup.gointernal/app/backup_test.gointernal/app/restore.gointernal/app/restore_chain_test.gointernal/app/roundtrip_test.gointernal/app/verify.gointernal/cli/dumps.gointernal/cli/root.gointernal/config/config.gointernal/config/storage_test.gointernal/dumps/catalog.gointernal/dumps/catalog_test.gointernal/dumps/chain.gointernal/dumps/chain_test.gointernal/dumps/prune.gointernal/storage/local.gointernal/storage/local_test.gointernal/storage/s3.gointernal/storage/s3_integration_test.gointernal/storage/store.gointernal/storage/suite.gointernal/tui/dashboard_test.gointernal/tui/modals/restore.gointernal/tui/panels/dumps.go
- Apply post-review fixes to the cloud-storage cycle. - local Put: fsync the temp file and the parent dir around the rename so a dump that survives Put is durable across a crash (the Store contract promises durability and this is a backup tool). - local Put: replace os.Rename with atomicRename, which removes the destination and retries where rename-over-existing can fail (Windows), honoring the overwrite contract across filesystems. - Catalog.List: propagate system/cancellation errors instead of skipping every ReadMeta failure; only skip CodeUser (corrupt/missing) entries, so a backend outage or cancelled context is not hidden as partial success. - Catalog.Delete: remove the meta object first, then the body, so a partial failure leaves at worst an invisible orphan body rather than a catalog entry pointing at a deleted body (mirrors meta-last write). - config Validate: trim whitespace on type/bucket so a blank-but- nonempty value fails fast instead of at runtime. - TUI restore modal and dumps panel: bound the catalog List with a 5s timeout so a slow/unreachable S3 backend cannot hang the UI. - store.go: fix the ErrNotFound doc to match the contract (Get returns it; Delete/Stat treat a missing key as a non-error). - Tests: check the previously-ignored ReadDir error in the atomic- publish test and the ReadAll/Stat errors in the contract suite.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/storage/local.go`:
- Around line 88-91: The fallback in atomicRename is too broad: it removes dst
after any os.Rename failure, which can delete valid data on unrelated errors.
Update the rename retry logic in atomicRename to only attempt removing dst when
the failure specifically indicates the destination already exists or cannot be
replaced, and otherwise return the original error immediately. Keep the
retry/remove behavior guarded by the existing os.Rename and os.Remove flow so
the overwrite path is only used for the intended case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4905e6dd-2459-41d7-860e-7a6e9b76b815
📒 Files selected for processing (8)
internal/config/config.gointernal/dumps/catalog.gointernal/storage/local.gointernal/storage/local_test.gointernal/storage/store.gointernal/storage/suite.gointernal/tui/modals/restore.gointernal/tui/panels/dumps.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/storage/store.go
- internal/storage/local_test.go
- internal/storage/suite.go
- internal/config/config.go
- internal/dumps/catalog.go
- The prior atomicRename removed the destination and retried on ANY os.Rename error. A transient failure unrelated to the destination existing (I/O glitch, permissions, device busy) would then delete the existing valid dump and still fail — destroying the last good copy. - Guard the remove+retry to fire only when the rename error matches fs.ErrExist (the destination-exists case on Windows / some filesystems); any other error returns untouched, leaving dst intact. - Add TestAtomicRename_PreservesDstOnUnrelatedError: a rename from a missing source must leave the existing destination unharmed.
Summary by CodeRabbit
storage:configuration block (bucket, prefix, region, endpoint).STORAGE.mdcovering storage configuration, guarantees, and verification behavior.